If we continue down this path (which I think we should) we should consider doing a few things:

• create a shared tsconfig by creating a TypeScript configuration package. The Turborepo docs have some ideas on how to do this. This will prevent us from duplicating tsconfig.json setups everywhere.
  • We actually have an old issue for this: Setup shared TypeScript configurations for monorepo packages #1295
• we should also consider using npm workspaces: Setup npm workspaces for shared code #1291
  • We can wait until we ditch Go, and can raise the extension to the top level of the repo

I think its a good idea to have separate packages. It makes the way we import and deal with some of the code around the homeView much easier to deal with. Right now it imports types from extension/vscode by importing with ../../.. which is not great.

@dotNomad would you say creating a shared configuration package should be part of this PR, or part of follow-up work?

Follow-up for sure. Just mentioning it here to spread awareness

I would prefer to keep using axios despite some of the problems we've had so we don't have to write our own error handling logic, or change how we do it in a lot of spots. That would simplify what we have here quite a bit.

This is a preference in design, but in my opinion it has a lot of benefits:

I would prefer to return the exact thing the API call responds with, including all data, and not creating new objects.

This will keep this code extremely single use and the only thing we will need to adjust overtime are the types. We won't have to adjust the functions since they just pass the response right through. We can always extract the data from the response.

It is much more flexible.

You can read more in our README.md for the Go API client we wrote: https://github.com/posit-dev/publisher/blob/183abd7d744fb8f6d8bcffc323717547521eca57/extensions/vscode/src/api/README.md

I see this is mapping a UserDTO to a User, but I think we should avoid this. It would make the initial migration easier, but once it is complete it will be much more difficult to understand why the attributes are different.

Down to discuss temporary trade-offs.

Branded Types are a nice addition to avoid ID overlap 👍

once we get rid of the go API client, we can clean up the shape of this stub, or just use the ConnectAPI client directly in test.

Right now the test shape is coupled to the go client's shape so we can have an apples-to-apples comparison in the contract tests, but we won't always need to have the apples-to-apples comparison we're running here.

Should we add a clean-up comment for more inline-context?

I could be convinced that the right approach will end up being splitting these out into atomic calls - maybe something we can feel out as we go along.

not strictly a light wrapper around an API call, but this does seem pretty useful to have on the API client.

Flagging again - not sure if we want to mimic this or not

I don't think so, but I'm fine starting with it to make migrations easier. Eventually I think it would be better to split this up.

We definitely should not do an await cascade like this though. We should make all of these calls in parallel with Promise.all() or something similar.

Right now this will wait for each individual API call to complete before making the next one severely slowing down this fetch.

@dotNomad is there any convention around calling something vs DTO?

I am not sure what you mean. Are you asking about a naming convention for API response objects?

Usually I don't distinguish types in that way, and just rely on plain names (no leading I for interfaces, or DTO signifier)

Could be a follow-up. We probably want to add this package to the dependabot groups we have to make keeping it up-to-date easier.

Should we add a clean-up comment for more inline-context?

Am I understanding this correctly that this will make every HTTP code pass? This is counter to how I'd expect to use Axios - I would expect non-200s to throw

https://axios-http.com/docs/handling_errors

My understanding is that this would let the caller interpret the HTTP code - which is how I'd expect to handle it in go, but if its better to throw I'm happy to do that.

It's the normal axios behavior to throw errors for non-200 error codes. If I was using an axios client I would definitely be surprised by that behavior.

I am a bit confused by this setup. Axios already has a client.request why wouldn't we use that by default?

I'm not seeing rawBody be used for example. Perhaps it would be clearer if that was setup in the individual API calls rather than in an abstract helper.

This also means as we used more axios features we'd have to expand this each time.

Similar note here about requestJson

I'm not sure what this is getting us.

We are needing to use as here a lot because we aren't typing the return from request. We ideally should be using an axios client get call and use the generic to type this for us to remove a lot of this as type assertions.

ah! I see - yeah that makes sense to me. Making the swap to use generics now.

Why are we doing this instead of letting the request throw default axios behavior bubble up?

I think we should avoid returning the data and instead type the request with a generic so we can get the response returned that has data inside of it.

Is the same as a while true?

they're semantically equivalent, yeah. Changing it to while (true) if that improves readability.

Is this why we don't use the default axios throw logic? Can we make that a one off for this API call so we don't have to have so many error catching blocks?

The amount of TypeScript type coercion happening here makes me a bit worried. I think we can simplify a bit using axios throw behavior, but catching it here as opposed to letting it throw up like the other methods to give us those custom Errors.

async testAuthentication(): Promise<UserDTO> {
    let data: UserDTO;
    try {
      ({ data } = await this.client.get<UserDTO>("/__api__/v1/user"));
    } catch (err) {
      if (axios.isAxiosError(err)) {
        const msg = (err.response?.data as Record<string, unknown>)?.error;
        throw new Error(typeof msg === "string" ? msg : `HTTP ${err.response?.status}`);
      }
      throw err;
    }
    // custom erroring

We can do something like this which is less fragile and custom.

This is the same as latestBundleId. Perhaps we simplify and remove latestBundleId

This looks like a typo. We are renaming the __api__/server_settings return, but perhaps we use a key like serverSettings. Caps is a bit strange.

Note here about the odd casing.

I don't think so, but I'm fine starting with it to make migrations easier. Eventually I think it would be better to split this up.

We definitely should not do an await cascade like this though. We should make all of these calls in parallel with Promise.all() or something similar.

Right now this will wait for each individual API call to complete before making the next one severely slowing down this fetch.

We can simplify this to client.get which is a bit more concise and easier to read IMO

I think we can make a factory function to assist with reducing the number of type coercions we use in here. Very minor nit though.

We should use axios params to fill this out.

axios handles encoding and is more close to how axios wants to be used.